Feature: Handle symbol-to-proc wrappers (&:) that refer to a method created using delegation or method missing#406
Conversation
… created using delegation or method missing These methods have an arity of -1
marcrohloff
force-pushed
the
feature/support-ampersand-blocks-for-delegated-methods
branch
from
2b8851e to
be995c1
Compare
Danger ReportNo issues found. |
numbata
left a comment
numbata
left a comment
There was a problem hiding this comment.
@marcrohloff Thanks for the contribution! The motivation is solid. 👍
Two things to address before merging: the arity <= 0 guard is too wide (see inline comment), and the new method_missing in the shared fixture risks making the "undefined method" test fragile. Left suggestions on both.
| arity = object.method(origin_method_name).arity | ||
| return if arity.zero? | ||
| # functions defined using `delegate` or `method_missing` have an arity of -1 | ||
| return if arity <= 0 |
There was a problem hiding this comment.
This is too permissive and reintroduces the bug class #389 was meant to prevent. Ruby's negative arities encode -(required + 1), so def foo(a, *rest) has arity -2 and def foo(a, b, *rest) has arity -3 - both have required positional
args, but both now sail through this guard and blow up later at instance_exec(object, &block) with a "wrong number of arguments" raised from inside user code, instead of the friendly ArgumentError this method was designed to produce.
Worth noting: def method_missing(method, ...) itself has arity -2, so even the stated motivating case is partially mishandled by <= 0.
Suggested fix is match the actual intent ("no required positional args"):
arity = object.method(origin_method_name).arity
required = arity >= 0 ? arity : -arity - 1
return if required.zero?| @@ -1,5 +1,7 @@ | |||
| ### Next Release | |||
|
|
|||
| * [#406](https://github.com/ruby-grape/grape-entity/pull/406): Handle symbol-to-proc wrappers (`&:method_name`) where the method uses `delegate` or `method_missing` - [@marcrohloff](https://github.com/marcrohloff). | |||
There was a problem hiding this comment.
The entry is placed above the #### Features / #### Fixes headers rather than under one of them. Please move the line under #### Fixes.
| Ruby style is enforced with [Rubocop](https://github.com/bbatsov/rubocop), run `bundle exec rubocop` and fix any style issues highlighted. | ||
|
|
||
| Make sure that `bundle exec rake` completes without errors. | ||
| Make sure that `bundle exec rake` and `rubocop` completes without errors. |
There was a problem hiding this comment.
| Make sure that `bundle exec rake` and `rubocop` completes without errors. | |
| Make sure that `bundle exec rake` and `rubocop` complete without errors. |
Subject-verb agreement: two subjects -> complete. Also the line above already instructs users to run bundle exec rubocop, so this becomes a near-duplicate 🤷
| subject.expose :using_missing, &:method_using_missing | ||
|
|
||
| object = SomeObject.new | ||
| expect(object.method(:method_using_missing).arity).to eq(-1) |
There was a problem hiding this comment.
The value assertion is the one that proves grape-entity handles it correctly. The arity line is testing Ruby.
| expect(object.method(:method_using_missing).arity).to eq(-1) |
| raise ArgumentError, 'something different' | ||
| end | ||
|
|
||
| def method_missing(method, ...) |
There was a problem hiding this comment.
Rather than adding method_missing directly to SomeObject, could you extract the new fixtures into subclasses? That way the shared class stays clean and the existing tests (especially the "undefined method" one) don't depend on your
respond_to_missing? being correct to pass.
Something like:
SomeObjectWithMissing = Class.new(SomeObject) do
def method_missing(method, ...)
return 'missing-result' if method == :method_using_missing
super
end
def respond_to_missing?(method, include_private = false)
method == :method_using_missing || super
end
end
SomeObjectWithDelegation = Class.new(SomeObject) do
InnerDelegate = Class.new { def delegated_method = 'delegated-result' }
delegate :delegated_method, to: :inner
def inner = @inner ||= InnerDelegate.new
endand specs kind of:
context 'with &: referencing a method_missing-backed method' do
specify do
subject.expose :using_missing, &:method_using_missing
object = SomeObjectWithMissing.new
expect(subject.represent(object).value_for(:using_missing)).to eq('missing-result')
end
end
context 'with &: referencing a delegated method' do
specify do
subject.expose :using_delegation, &:delegated_method
object = SomeObjectWithDelegation.new
expect(subject.represent(object).value_for(:using_delegation)).to eq('delegated-result')
end
endwith the regression case that pins the boundary:
context 'with &: referencing a method with required args and a splat (arity -2)' do
specify do
subject.expose :x, &:method_with_required_and_splat
object = SomeObjectWithMissing.new # or any object with such a method
expect do
subject.represent(object).value_for(:x)
end.to raise_error(ArgumentError, match(/method expects/))
end
end2 participants
These methods have a negative
arity